Skip to content

Prefer close backfill points (absolute distance)#19748

Merged
MadLittleMods merged 3 commits into
developfrom
madlittlemods/prefer-close-backfill-points
May 15, 2026
Merged

Prefer close backfill points (absolute distance)#19748
MadLittleMods merged 3 commits into
developfrom
madlittlemods/prefer-close-backfill-points

Conversation

@MadLittleMods

@MadLittleMods MadLittleMods commented May 1, 2026

Copy link
Copy Markdown
Contributor

Prefer close backfill points (absolute distance)

This isn't fixing any particular issue. It's just a follow-up I thought about after merging #19611 since we're now also dealing with backfill points in the nearby range ahead of the current_depth. And it's possible that the previous sort could bias to all nearby backfill points ahead of the current_depth that don't extend into the visible window of events we're paginating through.

Todo

  • Quick sanity check that the sort works with the tuple

Dev notes

Sort sanity check:

current_depth = 35
actual_sorted_list = sorted(
	[10, 20, 25, 30, 35, 40, 50],
	key=lambda depth: (
	    # Prefer backfill points that are closer to the `current_depth`
	    # (absolute distance)
	    abs(current_depth - depth),
	    # For the tie-break, we care about events that are actually in the past
	    # as they're more likely to reveal history that we can return (something
	    # absolutely in the past is better than something can potentially extend
	    # into the past).
	    #
	    # This sorts ascending so 0 sorts before 1
	    0 if current_depth >= depth else 1,
	),
)

expected_sorted_list = [35, 30, 40, 25, 20, 50, 10]

assert actual_sorted_list == expected_sorted_list, f"Expected actual list ({actual_sorted_list}) to be sorted like the expected list ({expected_sorted_list})"
print("Success")

Pull Request Checklist

  • Pull request is based on the develop branch
  • Pull request includes a changelog file. The entry should:
    • Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
    • Use markdown where necessary, mostly for code blocks.
    • End with either a period (.) or an exclamation mark (!).
    • Start with a capital letter.
    • Feel free to credit yourself, by adding a sentence "Contributed by @github_username." or "Contributed by [Your Name]." to the end of the entry.
  • Code style is correct (run the linters)

#
# This sorts ascending so 0 sorts before 1
0 if current_depth >= e.depth else 1,
),

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No tests but I sanity checked this sort with the snippet in the PR description

)

# If we have no backfill points lower than the `current_depth` then either we
# If we have no backfill points lower than the `nearby_depth` then either we

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not exactly related to this PR but just a little correction since #19611 was merged. nearby_depth refers to the nearby_depth parameter we assemble above when calling get_backfill_points_in_room(...)

@MadLittleMods MadLittleMods marked this pull request as ready for review May 1, 2026 20:37
@MadLittleMods MadLittleMods requested a review from a team as a code owner May 1, 2026 20:37

@anoadragon453 anoadragon453 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems sane to me. Clever use of a tuple to tie-break the sorting!

@MadLittleMods MadLittleMods merged commit 19f6362 into develop May 15, 2026
79 of 81 checks passed
@MadLittleMods MadLittleMods deleted the madlittlemods/prefer-close-backfill-points branch May 15, 2026 16:49
@MadLittleMods

Copy link
Copy Markdown
Contributor Author

Thanks for the review @anoadragon453 🐀

FrenchGithubUser pushed a commit to famedly/synapse-upstreaming that referenced this pull request Jun 12, 2026
This isn't fixing any particular issue. It's just a follow-up I thought
about after merging element-hq#19611
since we're now also dealing with backfill points in the nearby range
ahead of the `current_depth`. And it's possible that the previous sort
could bias to all nearby backfill points ahead of the `current_depth`
that don't extend into the visible window of events we're paginating
through.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants